feat: Add Gitea commit endpoints (getCommit, getLatestCommit)#66
feat: Add Gitea commit endpoints (getCommit, getLatestCommit)#66jaysomani wants to merge 5 commits intoutopia-php:mainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughTwo public methods were added to the Gitea VCS adapter: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/VCS/Adapter/Git/Gitea.php`:
- Line 410: The branch value is interpolated directly into the commit URL and
can contain query-significant characters; update the URL construction in
Gitea.php so the branch is URL-encoded (e.g., replace "{$branch}" with
rawurlencode($branch)) or build the query with
http_build_query(['sha'=>$branch,'limit'=>1]) and append that to
"/repos/{$owner}/{$repositoryName}/commits" so $url is safe; modify the
assignment to $url accordingly (referencing the $url variable and $branch).
In `@tests/VCS/Adapter/GiteaTest.php`:
- Around line 420-438: The two tests testGetCommitWithInvalidSha and
testGetLatestCommitWithInvalidBranch create repositories via
$this->vcsAdapter->createRepository(...) but never delete them on the exception
path; wrap the assertions that call $this->vcsAdapter->getCommit(...) and
$this->vcsAdapter->getLatestCommit(...) in try/finally blocks that always call
the cleanup method (e.g. $this->vcsAdapter->deleteRepository(self::$owner,
$repositoryName)) in the finally so the repositoryName created in each test is
removed regardless of the thrown exception.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d54667d93b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Adds commit-retrieval support to the Gitea VCS adapter to align with the existing GitHub adapter, along with new integration tests that exercise commit fetching and error paths.
Changes:
- Implement
getCommit(owner, repo, commitHash)in the Gitea adapter. - Implement
getLatestCommit(owner, repo, branch)in the Gitea adapter. - Add PHPUnit integration tests covering commit retrieval and invalid SHA/branch handling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/VCS/Adapter/Git/Gitea.php |
Adds Gitea implementations for getCommit and getLatestCommit. |
tests/VCS/Adapter/GiteaTest.php |
Adds integration tests validating commit retrieval and error handling scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/VCS/Adapter/GiteaTest.php (1)
366-390: Always guarantee repository cleanup in happy-path commit tests.These tests delete repositories only at the end. If an assertion or API call fails earlier, cleanup is skipped and can leak test resources across runs. Wrap test bodies in
try/finallyand keepdeleteRepository(...)infinally.♻️ Suggested pattern
public function testGetCommit(): void { $repositoryName = 'test-get-commit-' . \uniqid(); $this->vcsAdapter->createRepository(self::$owner, $repositoryName, false); - $this->vcsAdapter->createFile(self::$owner, $repositoryName, 'README.md', '# Test Commit'); - - $latestCommit = $this->vcsAdapter->getLatestCommit(self::$owner, $repositoryName, 'main'); - $commitHash = $latestCommit['commitHash']; - - $result = $this->vcsAdapter->getCommit(self::$owner, $repositoryName, $commitHash); - - $this->assertIsArray($result); - $this->assertArrayHasKey('commitHash', $result); - $this->assertArrayHasKey('commitMessage', $result); - $this->assertArrayHasKey('commitAuthor', $result); - $this->assertArrayHasKey('commitUrl', $result); - $this->assertArrayHasKey('commitAuthorAvatar', $result); - $this->assertArrayHasKey('commitAuthorUrl', $result); - - $this->assertSame($commitHash, $result['commitHash']); - $this->assertNotEmpty($result['commitMessage']); - - $this->vcsAdapter->deleteRepository(self::$owner, $repositoryName); + try { + $this->vcsAdapter->createFile(self::$owner, $repositoryName, 'README.md', '# Test Commit'); + $latestCommit = $this->vcsAdapter->getLatestCommit(self::$owner, $repositoryName, 'main'); + $commitHash = $latestCommit['commitHash']; + $result = $this->vcsAdapter->getCommit(self::$owner, $repositoryName, $commitHash); + + $this->assertIsArray($result); + $this->assertArrayHasKey('commitHash', $result); + $this->assertArrayHasKey('commitMessage', $result); + $this->assertArrayHasKey('commitAuthor', $result); + $this->assertArrayHasKey('commitUrl', $result); + $this->assertArrayHasKey('commitAuthorAvatar', $result); + $this->assertArrayHasKey('commitAuthorUrl', $result); + $this->assertSame($commitHash, $result['commitHash']); + $this->assertNotEmpty($result['commitMessage']); + } finally { + $this->vcsAdapter->deleteRepository(self::$owner, $repositoryName); + } }Also applies to: 392-414, 444-457
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/VCS/Adapter/GiteaTest.php` around lines 366 - 390, Wrap the body of the testGetCommit method in a try/finally so deleteRepository(self::$owner, $repositoryName) always runs; specifically, after creating $repositoryName and before any assertions/calls (i.e., around calls to createFile, getLatestCommit, getCommit and assertions), place the assertions and API calls in the try block and move the existing deleteRepository call into the finally block to guarantee cleanup even if an assertion or API call throws. Apply the same try/finally pattern to the other tests referenced (lines noted: the tests around 392-414 and 444-457) that currently call deleteRepository only at the end.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/VCS/Adapter/GiteaTest.php`:
- Around line 366-390: Wrap the body of the testGetCommit method in a
try/finally so deleteRepository(self::$owner, $repositoryName) always runs;
specifically, after creating $repositoryName and before any assertions/calls
(i.e., around calls to createFile, getLatestCommit, getCommit and assertions),
place the assertions and API calls in the try block and move the existing
deleteRepository call into the finally block to guarantee cleanup even if an
assertion or API call throws. Apply the same try/finally pattern to the other
tests referenced (lines noted: the tests around 392-414 and 444-457) that
currently call deleteRepository only at the end.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/VCS/Adapter/Git/Gitea.phptests/VCS/Adapter/GiteaTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/VCS/Adapter/Git/Gitea.php
Meldiron
added
test
| $this->assertArrayHasKey('commitAuthorUrl', $result); | ||
|
|
||
| $this->assertNotEmpty($result['commitHash']); | ||
| $this->assertNotEmpty($result['commitAuthor']); |
There was a problem hiding this comment.
Same comment as above - assertHasKey and assertEmpty are most minimal assertions - everytime we can assert value, we should.
For example here I would assertSame for commit hash, message, and author (author only if possible, not sure)
tests/VCS/Adapter/GiteaTest.php
Outdated
| public function testGetCommitVerifyMessageContent(): void | ||
| { | ||
| $repositoryName = 'test-commit-message-' . \uniqid(); | ||
| $this->vcsAdapter->createRepository(self::$owner, $repositoryName, false); | ||
|
|
||
| $customMessage = 'Custom commit message'; | ||
| $this->vcsAdapter->createFile(self::$owner, $repositoryName, 'README.md', '# Test', $customMessage); | ||
|
|
||
| $latestCommit = $this->vcsAdapter->getLatestCommit(self::$owner, $repositoryName, 'main'); | ||
|
|
||
| $this->assertStringContainsString($customMessage, $latestCommit['commitMessage']); | ||
|
|
||
| $this->vcsAdapter->deleteRepository(self::$owner, $repositoryName); |
There was a problem hiding this comment.
I think we can remove this test, and ensure message (as well as author maybe) in other tests.
Also assertStringContainsString is a bit loose, I would go for assertSame.
--
I think you did this after our discussion about separating tests. Over-separation can be problem too - what helps me decide in most scenarios is to always look from perspective of action I am doing in the test, not from perspective of assertions.
Here, action in same as one in testGetLatestCommit - preparing a file with a message in commit. So I would keep it as part of same test.
If there was some extra setup (like more repositories, or more commits), or the flow had multiple steps (like ensuring it's 2nd createFile, not 1st), thats when separate test would make sense to me.
Open to discuss this further if you have strong opinion on this. I am going off of my experience with testing, so I might be missing something too
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/VCS/Adapter/Git/Gitea.php (2)
422-424: Split HTTP-failure vs payload-shape errors.Line 422 currently throws the same message for both transport/status failures and malformed/empty payloads, which makes debugging harder.
🛠️ Proposed improvement
- if ($statusCode >= 400 || empty($response['body'][0])) { - throw new Exception("Latest commit response is missing required information."); - } + if ($statusCode >= 400) { + throw new Exception("Failed to fetch latest commit for branch '{$branch}': HTTP {$statusCode}"); + } + if (empty($response['body'][0])) { + throw new Exception("Latest commit response is missing required information."); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/VCS/Adapter/Git/Gitea.php` around lines 422 - 424, The current check conflates HTTP transport/status errors and malformed payloads; update the logic in Gitea (the method using $statusCode and $response) to throw two distinct exceptions: if ($statusCode >= 400) throw an Exception that includes the HTTP status code and response body/details (e.g. "Failed to fetch latest commit: HTTP {status} - {body}"), otherwise if (empty($response['body'][0])) throw a different Exception clearly stating the payload is missing required commit info (e.g. "Latest commit response missing required information"), so callers can distinguish transport errors from payload-shape errors.
391-391: Remove unused local$committer.Line 391 assigns
$committerbut it is never used in the return statement, which adds noise and triggers static-analysis warnings.♻️ Proposed cleanup
- $committer = $body['committer'] ?? [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/VCS/Adapter/Git/Gitea.php` at line 391, The local variable $committer in src/VCS/Adapter/Git/Gitea.php is assigned but never used; remove the unnecessary assignment "$committer = $body['committer'] ?? [];" (or, if committer data is actually required, include it in the return structure where other $body fields are returned) to eliminate dead code and static-analysis warnings—look for the assignment in the Git/Gitea adapter function that processes the incoming $body and either delete that line or wire $committer into the returned payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/VCS/Adapter/Git/Gitea.php`:
- Around line 422-424: The current check conflates HTTP transport/status errors
and malformed payloads; update the logic in Gitea (the method using $statusCode
and $response) to throw two distinct exceptions: if ($statusCode >= 400) throw
an Exception that includes the HTTP status code and response body/details (e.g.
"Failed to fetch latest commit: HTTP {status} - {body}"), otherwise if
(empty($response['body'][0])) throw a different Exception clearly stating the
payload is missing required commit info (e.g. "Latest commit response missing
required information"), so callers can distinguish transport errors from
payload-shape errors.
- Line 391: The local variable $committer in src/VCS/Adapter/Git/Gitea.php is
assigned but never used; remove the unnecessary assignment "$committer =
$body['committer'] ?? [];" (or, if committer data is actually required, include
it in the return structure where other $body fields are returned) to eliminate
dead code and static-analysis warnings—look for the assignment in the Git/Gitea
adapter function that processes the incoming $body and either delete that line
or wire $committer into the returned payload.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/VCS/Adapter/Git/Gitea.phptests/VCS/Adapter/GiteaTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/VCS/Adapter/GiteaTest.php
Changes
Implements commit-related endpoints for Gitea adapter to match GitHub functionality.
Methods Implemented
getCommit(owner, repo, commitHash)- Fetches details of a specific commit by SHAgetLatestCommit(owner, repo, branch)- Fetches the most recent commit of a branchTests Added
testGetCommit- Verifies commit details retrievaltestGetLatestCommit- Verifies latest commit fetchingtestGetCommitWithInvalidSha- Tests error handling for invalid commit SHAtestGetLatestCommitWithInvalidBranch- Tests error handling for non-existent branchtestGetCommitVerifyMessageContent- Verifies custom commit messages are preservedAll tests follow the established pattern with proper setup/teardown and edge case coverage.
Summary by CodeRabbit
New Features
Tests